Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: flip the sign for balanceDelta #49

Merged
merged 4 commits into from
May 29, 2024

Conversation

chefburger
Copy link
Collaborator

According to our internal discussion, we've flipped the sign for BalanceDelta to make it less counter-intuitive.

  1. Before: positive amount means the caller owes the vault, negative while positive amount means the vault owes the caller
  2. After: negative amount means the caller owes the vault, while positive amount means the vault owes the caller

@chefburger chefburger force-pushed the refactor/flip-balanceDelta-sign branch from 267043f to 4fdc4cb Compare May 28, 2024 04:34
@@ -163,9 +163,11 @@ library BinHooks {
hookDeltaUnspecified += beforeSwapDelta.getUnspecifiedDelta();

if (hookDeltaUnspecified != 0 || hookDeltaSpecified != 0) {
// the hookDelta is the delta added into amountIn which is in the opposite direction of the balanceDelta
// i.e. in a reasonable case, amountIn will basiaclly equal to "-BalanceDelta.amount0()/amount1()"
Copy link
Collaborator Author

@chefburger chefburger May 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is now completely opposite when comparing with cl-pool because for cl-pool when amountSpecified > 0 it means we are specifying amountOut, while for bin-pool we can only specify amountIn and it's always positive.

That's why in the following call i.e. toBalanceDelta we flip the sign for both parts

Copy link
Collaborator

@ChefMist ChefMist May 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

want to clarify: does the hook for both Bin and CL have the same behavour for their return value? eg. BeforeSwapDelta in beforeSwap and int128 at afterSwap()?

eg.

1/ what does negative BeforeSwapDelta for both value means?
2/ what does negative int128 in afterSwap means?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this right?

1/ what does negative BeforeSwapDelta for both value means?

  1. if beforeSwapDelta is both negative, it means the hook want to give user extra tokenIn and tokenOut
  2. if beforeSwapDelta is positive, it means the hook want to take a "fee" in both tokenIn and tokenOut
  3. likewise if its (positive , negative) it means hook want to take tokenIn as fee and give extra tokenOut

2/ what does negative int128 in afterSwap means?

  1. if int128 is negative, it means the hook want to give amountOut
  2. if int128 is positive, it means the hook want to take some amountOut as "fee"

Copy link
Collaborator Author

@chefburger chefburger May 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for bin-pool, I think it should be opposite.

  1. if beforeSwapDelta is both negative, it means the hook want to take a "fee" in both tokenIn and tokenOut
  2. if beforeSwapDelta is positive, it means the hook want to give user extra tokenIn and tokenOut
  3. likewise if its (positive , negative) it means hook want to give extra tokenIn and take tokenOut as fee

Let's elaborate case 3 for example:

if BeforeSwapDelta.getSpecifiedDelta() > 0, i.e. amountToSwap is even bigger than trader's specification, so more amountIn needed for this swap, and let's see who will pay for the extra part:

from trader perspective:

The amountIn delta returned from pool.swap is definitely bigger in abs value because amountIn is bigger, but remember the delta is updated through delta - hookDelta in afterSwap

and we know hookDelta = (-hookDeltaSpecified, -hookDeltaUnspecified)
a negative value minus another negative value will result in a bigger negative value i.e. user are actually not paying that part

from hook perspective:

if (hookDelta != BalanceDeltaLibrary.ZERO_DELTA) {
    vault.accountPoolBalanceDelta(key, hookDelta, address(key.hooks));
}

This will count a negative amountIn delta on hook, so inside hook callback there must be a payment to the pool

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2/ what does negative int128 in afterSwap means?

Also, should be opposite
if int128 is positive, it means the hook want to give amountOut
if int128 is negative, it means the hook want to take some amountOut as "fee"

In fact
int128 in afterSwap and BeforeSwapDelta.getUnspecifiedDelta() in beforeSwap are actually specifying the same thing..

can see they are combined in afterSwap before generating the final hookDelta:

CleanShot 2024-05-28 at 15 14 01@2x

if (amount == 0) amount = SettlementGuard.getCurrencyDelta(target, currency).toUint256();
SettlementGuard.accountDelta(msg.sender, currency, amount.toInt128());
SettlementGuard.accountDelta(target, currency, -(amount.toInt128()));
/// It will revert if target has positive delta
Copy link
Collaborator

@ChefMist ChefMist May 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

curious, what does this comment mean. does it mean that it will revert eventually in the transaction if the target has non-zero delta?

eg. the comment might imply that if target has positive delta, this method will revert

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when calling settleFor, msg.sender is trying to cover the payment for target right ?

If the target has a positive delta, it means that the target doesn't owe anything to the pool and, in fact, the pool owes tokens to them..(after updating everything flip) In this case, pass in amount=0 (i.e. ettle all outstanding debt for target) is invalid.

Copy link
Collaborator

@ChefMist ChefMist May 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when calling settleFor, msg.sender is trying to cover the payment for target right ?

I guess when there's a comment like /// It will revert if target has positive delta -- people might think there's a revert in this settleFor method. But its basically the CurrencyNotSettled() revert that happens at the end right? or is there actually a validation for this target has positive delta specifically?

If the comment is correct, would it mean we can write the same comment in take/settle etc.. it will revert if caller has positive delta as well?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

“But its basically the CurrencyNotSettled() revert that happens at the end right? or is there actually a validation for this target has positive delta specifically?”

No, it will revert as in SafeCast.toUint256 because we cant cast a negative number (given delta is positive, minus it results in negative) to uint256

CleanShot 2024-05-28 at 15 58 06@2x

@@ -171,7 +171,7 @@ interface IBinPoolManager is IProtocolFees, IPoolManager, IExtsload {
returns (BalanceDelta delta);

/// @notice Donate the given currency amounts to the pool with the given pool key.
/// @return delta Positive amt means the caller owes the vault, while negative amt means the vault owes the caller
/// @return delta Negative amt means the caller owes the vault, while positive amt means the vault owes the caller
/// @return binId The donated bin id, which is the current active bin id. if no-op happen, binId will be 0
function donate(PoolKey memory key, uint128 amount0, uint128 amount1, bytes calldata hookData)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there's also the mint() comment around delta that neeed to flipped

 /// @return delta BalanceDelta, will be positive indicating how much total amt0 and amt1 liquidity added

to

 /// @return delta BalanceDelta, will be negative indicating how much total amt0 and amt1 liquidity added

ChefMist
ChefMist previously approved these changes May 29, 2024
Copy link
Collaborator

@ChefMist ChefMist left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

approved -- can you please address https://github.com/pancakeswap/pancake-v4-core/pull/49/files#r1616613856 before merging, seems like the comment for mint() is not updated

though per discussed -- need to look if we can fix the current discrepancy in beforeSwapDelta between CL and Bin pool in next PR

@chefburger
Copy link
Collaborator Author

approved -- can you please address https://github.com/pancakeswap/pancake-v4-core/pull/49/files#r1616613856 before merging, seems like the comment for mint() is not updated

though per discussed -- need to look if we can fix the current discrepancy in beforeSwapDelta between CL and Bin pool in next PR

sorry. Forgot to push the update, can u check again

@chefburger
Copy link
Collaborator Author

@ChefSnoopy @chef-omelette Feel free to add more comments, i will merge for now to unblock incoming optimization

@chefburger chefburger merged commit 4336d26 into main May 29, 2024
2 checks passed
@chefburger chefburger deleted the refactor/flip-balanceDelta-sign branch May 29, 2024 06:35
@chefburger
Copy link
Collaborator Author

chefburger commented May 29, 2024

Can also take a look at the following one (check #50) which alters the logic regarding how we deal with before/afterSwap return value for bin-poo to keep it consistent across pool types

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants